Skip to content

Conversation

dimpase
Copy link
Member

@dimpase dimpase commented Aug 9, 2025

also fix a typo in spkg-configure.m4 of roman_numerals_py, and fix spkg-configure.m4 of flint to allow versions 3.3.* (will be done elsewhere - in #40568)

📝 Checklist

  • The title is concise and informative.
  • The description explains in detail what this PR is about.
  • I have linked a relevant issue or discussion.
  • I have created tests covering the changes.
  • I have updated the documentation and checked the documentation preview.

⌛ Dependencies

@dimpase
Copy link
Member Author

dimpase commented Aug 9, 2025

@tobiasdiez - not sure, is the following the good fix for the meson build:

--- a/src/sage/libs/meson.build
+++ b/src/sage/libs/meson.build
@@ -1,4 +1,3 @@
-sirocco = cc.find_library('sirocco', required: false, disabler: true)
 # cannot be found via pkg-config
 ecl = cc.find_library('ecl', required: false, disabler: true)
 if not ecl.found() and not is_windows
@@ -65,6 +64,12 @@ else
   conf_data.set('SAGE_MAXIMA', maxima_bin.full_path())
 endif
 
+sirocco = dependency(
+  'libsirocco',
+  version: '>=2.1.0',
+  required: false,
+  disabler: true,
+)
 braiding = dependency(
   'libbraiding',
   version: '>=1.3.1',

@dimpase
Copy link
Member Author

dimpase commented Aug 9, 2025

sirocco tarball is an unreleased version I made from my Sirocco PR miguelmarco/SIROCCO2#7

Copy link

github-actions bot commented Aug 9, 2025

Documentation preview for this PR (built with commit a6827d0; changes) is ready! 🎉
This preview will update shortly after each push to this PR.

@kiwifb
Copy link
Member

kiwifb commented Aug 9, 2025

Is the .pc file needed for the meson build?

@dimpase
Copy link
Member Author

dimpase commented Aug 9, 2025

Is the .pc file needed for the meson build?

yes, and I checked that with the last commit meson build of sirocco interface works

@antonio-rojas
Copy link
Contributor

@tobiasdiez - not sure, is the following the good fix for the meson build:

--- a/src/sage/libs/meson.build
+++ b/src/sage/libs/meson.build
@@ -1,4 +1,3 @@
-sirocco = cc.find_library('sirocco', required: false, disabler: true)
 # cannot be found via pkg-config
 ecl = cc.find_library('ecl', required: false, disabler: true)
 if not ecl.found() and not is_windows
@@ -65,6 +64,12 @@ else
   conf_data.set('SAGE_MAXIMA', maxima_bin.full_path())
 endif
 
+sirocco = dependency(
+  'libsirocco',
+  version: '>=2.1.0',
+  required: false,
+  disabler: true,
+)
 braiding = dependency(
   'libbraiding',
   version: '>=1.3.1',

the meson build works just fine with the current release of sirocco (without pkgconfig). Please don't make it depend on an unofficial fork.

@dimpase
Copy link
Member Author

dimpase commented Aug 9, 2025

it's not an unofficial fork, it's just a temporary fix, until @miguelmarco merges my PR

@antonio-rojas
Copy link
Contributor

it's not an unofficial fork, it's just a temporary fix, until @miguelmarco merges my PR

Still, until it is merged and released this will break distros, including conda (you can see in the conda testers that sirocco is not detected). Please revert the meson bits, there is nothing broken that needs fixing there.

@dimpase
Copy link
Member Author

dimpase commented Aug 9, 2025

It didn't work for me without pkg-config for libsirocco in SAGE_LOCAL.

@tobiasdiez
Copy link
Contributor

You can first try via pkg-config and if that's not working fallback to find_library similar to how its done for gap:
https://github.com/sagemath/sage/blob/858268b40010e5ed6da13488ad0f52cda4d1f70e/src/meson.build#L191C1-L201C6

@orlitzky
Copy link
Contributor

orlitzky commented Aug 9, 2025

You can first try via pkg-config and if that's not working fallback to find_library similar to how its done for gap: https://github.com/sagemath/sage/blob/858268b40010e5ed6da13488ad0f52cda4d1f70e/src/meson.build#L191C1-L201C6

Might want to rebase over #40485 as well since it touches this line.

@dimpase
Copy link
Member Author

dimpase commented Aug 9, 2025

OK, I've reverted the commit touching meson.build. It should be not so controversial now.

@enriqueartal
Copy link
Contributor

I uninstalled both sirocco and sagemath_sirocco, applied the patch, ./configure --enable-sirocco, make build and got this:

┌────────────────────────────────────────────────────────────────────┐
│ SageMath version 10.7.rc2, Release Date: 2025-08-09                │
│ Using Python 3.13.5. Type "help()" for help.                       │
└────────────────────────────────────────────────────────────────────┘
┏━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┓
┃ Warning: this is a prerelease version, and it may be unstable.     ┃
┗━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┛
sage: A.<x, y> = AffinePlaneCurveArrangements(QQ)
sage: C = A(y^2 - x^3)
sage: C.fundamental_group()
Exception raised by child process with pid=457157:
Traceback (most recent call last):
  File "/usr/local/sage15/src/sage/parallel/use_fork.py", line 191, in __call__
    self._subprocess(f, dir, *v0)
    ~~~~~~~~~~~~~~~~^^^^^^^^^^^^^
  File "/usr/local/sage15/src/sage/parallel/use_fork.py", line 328, in _subprocess
    value = f(*args, **kwds)
  File "/usr/local/sage15/src/sage/schemes/curves/zariski_vankampen.py", line 898, in braid_in_segment
    aux = followstrand(f, [p for p in glist if p != f],
                       x0, x1, i.center(), precision1[f])
  File "/usr/local/sage15/src/sage/schemes/curves/zariski_vankampen.py", line 567, in followstrand
    from sage.libs.sirocco import (contpath, contpath_mp, contpath_comps, contpath_mp_comps)
ModuleNotFoundError: No module named 'sage.libs.sirocco'
---------------------------------------------------------------------------
AttributeError                            Traceback (most recent call last)
Cell In[3], line 1
----> 1 C.fundamental_group()

File /usr/local/sage15/src/sage/schemes/curves/plane_curve_arrangement.py:569, in AffinePlaneCurveArrangementElement.fundamental_group(self, simplified, vertical, projective)
    567 else:
    568     bd = None
--> 569 G, dic = fundamental_group_arrangement(L, simplified=simplified,
    570                                        puiseux=True,
    571                                        projective=projective,
    572                                        vertical=vertical,
    573                                        braid_data=bd)
    574 if simplified and vertical:
    575     self._fundamental_group_simpl_vertical = G

File /usr/local/sage15/src/sage/schemes/curves/zariski_vankampen.py:1902, in fundamental_group_arrangement(flist, simplified, projective, puiseux, vertical, braid_data)
   1900     d1 = 0
   1901 else:
-> 1902     bm, dic, dv, d1 = braid_monodromy(f, flist1, vertical=vertical)
   1903 vert_lines = list(dv)
   1904 vert_lines.sort()

File /usr/local/sage15/src/sage/schemes/curves/zariski_vankampen.py:1379, in braid_monodromy(f, arrangement, vertical)
   1377         b = braidcomputed[1]
   1378         segsbraids[(beginseg, endseg)] = b
-> 1379         segsbraids[(endseg, beginseg)] = b.inverse()
   1380     end_braid_computation = True
   1381 except ChildProcessError:  # hack to deal with random fails first time

AttributeError: 'str' object has no attribute 'inverse'

The same error without the patch. After ./configure --enable-sagemath_sirocco, make build:

sage: A.<x, y> = AffinePlaneCurveArrangements(QQ)
sage: C = A(y^2 - x^3)
sage: C.fundamental_group()
Finitely presented group < x0, x1 | x1*x0*x1*x0^-1*x1^-1*x0^-1 >

as it should be. I think the key is ModuleNotFoundError: No module named 'sage.libs.sirocco'.

@dimpase
Copy link
Member Author

dimpase commented Aug 10, 2025

if I do the latter, I get a non-working Sage, for some reason.

what you see is that enable-sirocco only builds the sirocco library, and the current build needs enable-sagemath_sirocco option to build the interface to sirocco.

in meson build it is automatically detected whether libsirocco is available, and the interface is built (but it is getting an option to control whether it's actually built)

@enriqueartal
Copy link
Contributor

Is there a way to build the interface to sirocco with --enable-sirocco?

@dimpase
Copy link
Member Author

dimpase commented Aug 10, 2025

start sage --buildsh and follow the detailed meson steps in installation/meson.rst

@enriqueartal
Copy link
Contributor

I am trying, but I would like to know if it is possible when building in the classical way.

@dimpase
Copy link
Member Author

dimpase commented Aug 11, 2025

it's not an unofficial fork, it's just a temporary fix, until @miguelmarco merges my PR

Still, until it is merged and released this will break distros, including conda (you can see in the conda testers that sirocco is not detected). Please revert the meson bits, there is nothing broken that needs fixing there.

@antonio-rojas - this is now based on the official new release.
I also too away unrelated changes, and don't touch meson. OK?

@dimpase dimpase requested a review from antonio-rojas August 11, 2025 04:00
@antonio-rojas
Copy link
Contributor

I can't test myself this month, but conda CI looks fine, so it should be ok on Arch too.

Copy link
Contributor

@tobiasdiez tobiasdiez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but perhaps someone actually using that stuff is a better reviewer ;-)

@enriqueartal
Copy link
Contributor

I am trying to build sage with meson for the first time to see if these changes work for me. I had some issues, see this message. If I get it I have some doubts:

  • How is an optional package installed with meson?
  • How can I force to use a sage package instead of a system one? For example, my flint system package is 3.2.2.

@dimpase
Copy link
Member Author

dimpase commented Aug 19, 2025

Please, this PR is a trivial update (apart from spkg-configure.m4, which you can only test on a system which has system-wide sirocco, or sirocco installed by hand in /usr/local or so)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants